Skip to content

Fix false negative when checking payload: false #1425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Conversation

roschaefer
Copy link
Contributor

Here is my example code:

    test('commits setContributionId', (done) => {
      const state = {
        comments: []
      }
      testAction(action, 42, state, [
        // ...
        { type: 'isLoading', payload: false}
      ], done)
    })

I'm passing 42 as an argument to testAction and I want to verify that commit('isLoading', payload) is executed. Here the payload is not the payload of the testAction helper (in my case 42) but the second argument of commit. That might be false and in my case it is. If I change the payload of the last expected commit to be true no assertion error is raised. The fix is to make the if condition in the mocked commit method more specific.

roschaefer and others added 2 commits October 16, 2018 19:41
Here is my example code:
```javascript
    test('commits setContributionId', (done) => {
      const state = {
        comments: []
      }
      testAction(action, 42, state, [
        // ...
        { type: 'isLoading', payload: false}
      ], done)
    })
```
I'm passing `42` as an argument to `testAction` and I want to verify that `commit('isLoading', payload)` is executed. Here the payload is not the payload of the testAction helper (in my case 42) but the second argument of `commit`. That might be `false` and my case it is. If I change the payload of the last expected commit to be `true` no assertion error is raised. The fix is to make the if condition in the mocked commit method more specific.
If a user specifies an expected payload, the assertion **must** be
executed.

Let's say your store action looks like this:

```
const somePayload = undefined
// this is the bug to be exposed, `somePayload` should not be undefined

commit('isLoading', somePayload)
```

And the developer defines an expected payload in the test:

```js
testAction(action, 42, state, [
  //...
  { type: 'isLoading', payload: false }
], done)
```

The test will unexpectedly pass as the assertion is skipped.
Instead we always want to check the assertion if any payload was
defined by the developer.
@roschaefer
Copy link
Contributor Author

I've added one more commit. From the commit message:

Given a mutation payload, don't skip assertion

If a user specifies an expected payload, the assertion must be
executed.

Let's say your store action looks like this:

const somePayload = undefined
// this is the bug to be exposed, `somePayload` should not be undefined

commit('isLoading', somePayload)

And the developer defines an expected payload in the test:

testAction(action, 42, state, [
  //...
  { type: 'isLoading', payload: false }
], done)

The test will unexpectedly pass as the assertion is skipped.
Instead we always want to check the assertion if any payload was
defined by the developer.

@ktsn
Copy link
Member

ktsn commented Oct 23, 2018

Maybe we don't need this if statement?

@kiaking kiaking added the documentation Improvements or additions to documentation label Apr 21, 2020
@kiaking
Copy link
Member

kiaking commented Apr 21, 2020

Yeah I think we can just remove if and be done with it. Always asserting the payload would be much more intuitive I think.

@roschaefer Do you think you'll be able to adjust the change?

@kiaking
Copy link
Member

kiaking commented Apr 21, 2020

@roschaefer Thanks a lot!

@kiaking kiaking merged commit 7da1f13 into vuejs:dev Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants